Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.1: Always enable nonce slot/owner-in-hash #10166

Closed
wants to merge 1 commit into from

Conversation

mvines
Copy link
Member

@mvines mvines commented May 21, 2020

With these two rolling updates now active on testnet, they should be always enabled on the v1.1 line in support of the v1.0 -> v1.1 migration of mainnet-beta

@mvines mvines force-pushed the on branch 2 times, most recently from 32ea6e8 to a8efaa9 Compare May 21, 2020 20:09
@mvines mvines added the automerge Merge this Pull Request automatically once CI passes label May 21, 2020
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label May 21, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@mvines mvines added the automerge Merge this Pull Request automatically once CI passes label May 21, 2020
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label May 21, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@ryoqun
Copy link
Member

ryoqun commented May 21, 2020

@mvines Oops. Maybe related to #10163, we can't do this... I'm removing auto-merge for now

@@ -1330,10 +1330,8 @@ impl AccountsDB {
hasher.result()
}

pub fn include_owner_in_hash(slot: Slot) -> bool {
Copy link
Member

@ryoqun ryoqun May 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvines Well, I've found we can't remove this gating until the cluster is over an epoch after eager rent collection is enabled.

That's because this slot is the slot a given account is updated at, not the slot when the bank hash is calculated for the bank at.... And accounts are at various slots. And accounts last updated older than 14_000_000 must be calculated with old hashing method (ie, don't include owner).

So we must wait to remove this for tds until eager rent collection updates all accounts...

This also means for mainnet-beta, we need similar gating still for the 1.1 branch for the cluster...

Maybe that's reason #10163 is occurring too; we need this logic for master to consume tds snapshots.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok cool. Happy to let this PR sit until Monday. I was just trying to get ahead of things for the v1.0 -> v1.1 mainnet-beta upgrade

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvines See #10163 (comment).

Sadly, we can't land this as-is....

I can take care of this. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure. The trick is that we're starting to run out of time. Here's what the release timeline looks like for the next week:

  1. master branch forks into the v1.2 branch today
  2. release v1.2.0 probably on Tuesday the 26th
  3. upgrade devnet to v1.2.0 ideally
  4. by the 29th, upgrade mainnet-beta to v1.1
  5. by the 29th, upgrade testnet to v1.2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll adjust the transition plan accordingly. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #10227 to track this change for a little later

@mvines
Copy link
Member Author

mvines commented May 22, 2020

hey @ryoqun, any idea why test_bad_bank_hash is failing here? Thanks!

@ryoqun
Copy link
Member

ryoqun commented May 22, 2020

hey @ryoqun, any idea why test_bad_bank_hash is failing here? Thanks!

@mvines Yeah, I'm aware of it. This is intended but too subtle....: We just need to change this line to true also: https://github.com/solana-labs/solana/pull/9918/files#diff-2099c5256db4eb5975c8834af38f6456R3513
That's because this pr is changing the new default hashing method (new == true).

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #10166 into v1.1 will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@          Coverage Diff           @@
##            v1.1   #10166   +/-   ##
======================================
  Coverage   80.6%    80.6%           
======================================
  Files        288      288           
  Lines      66085    66084    -1     
======================================
+ Hits       53308    53315    +7     
+ Misses     12777    12769    -8     

@@ -50,7 +50,7 @@ pub const SIZE_OF_NONCE_DATA_SHRED_PAYLOAD: usize =
pub const OFFSET_OF_SHRED_SLOT: usize = SIZE_OF_SIGNATURE + SIZE_OF_SHRED_TYPE;
pub const OFFSET_OF_SHRED_INDEX: usize = OFFSET_OF_SHRED_SLOT + SIZE_OF_SHRED_SLOT;
pub const NONCE_SHRED_PAYLOAD_SIZE: usize = PACKET_DATA_SIZE - SIZE_OF_NONCE;
pub const UNLOCK_NONCE_SLOT: Slot = 14_780_256;
pub const UNLOCK_NONCE_SLOT: Slot = 5_000_000;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this change into #10223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants